Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merging to release-5.5: [TT-12840, DX-1620] Added explanation of bug fix missed in 5.4 RN (#5260) #5286

Merged

Conversation

buger
Copy link
Member

@buger buger commented Aug 20, 2024

User description

[TT-12840, DX-1620] Added explanation of bug fix missed in 5.4 RN (#5260)

  • Added explanation of bug fix missed in 5.4 RN

PR Type

documentation, enhancement


Description

  • Added documentation explaining the corrected behavior for calculating effective rate limits when multiple policies are applied to a key.
  • Updated the release notes for version 5.4 to include details of the bug fix related to rate limit calculation.
  • Provided examples and notes to clarify the changes and their impact on existing policies.

Changes walkthrough 📝

Relevant files
Documentation
rate-limiting.md
Documented changes in rate limit calculation with multiple policies

tyk-docs/content/getting-started/key-concepts/rate-limiting.md

  • Added a section explaining the combination of multiple policies for
    rate limits.
  • Described the corrected behavior for calculating effective rate
    limits.
  • Included a note about the previous bug in rate limit calculation.
  • +14/-0   
    version-5.4.md
    Updated release notes with rate limit calculation fix       

    tyk-docs/content/product-stack/tyk-gateway/release-notes/version-5.4.md

  • Updated release notes to include a bug fix in rate limit calculation.
  • Detailed the previous and corrected behavior for rate limit
    determination.
  • Added a changelog entry explaining the fix.
  • +16/-1   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    )
    
    * Added explanation of bug fix missed in 5.4 RN
    
    (cherry picked from commit a63f1ba)
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    Copy link
    Contributor

    github-actions bot commented Aug 20, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Include a brief note on potential impacts on existing configurations in the summary

    It's beneficial to include a brief explanation of how the new logic might affect
    existing configurations directly in the summary to alert users who might not expand
    the detailed view.

    tyk-docs/content/product-stack/tyk-gateway/release-notes/version-5.4.md [302]

    -<summary>Fixed an error in the calculation of effective rate limit from multiple policies</summary>
    +<summary>Fixed an error in the calculation of effective rate limit from multiple policies. This change may affect existing configurations by applying a different rate limit.</summary>
     
    Suggestion importance[1-10]: 8

    Why: Including a note about potential impacts directly in the summary is important for alerting users who might not read the detailed view, ensuring they are aware of changes that could affect their configurations.

    8
    Add a direct link to the detailed release notes or documentation for the bug fix

    Consider adding a link to the specific release notes or documentation section that
    discusses the bug fix in more detail. This will provide readers with a direct
    reference to understand the changes and their implications fully.

    tyk-docs/content/getting-started/key-concepts/rate-limiting.md [84]

     {{< note success >}}
     **Note**  
    -Prior to Tyk 5.4.0 there was a long-standing bug in the calculation of the effective rate limit applied to the key where Tyk would combine the highest `rate` and highest `per` from the policies applied to the key, so for the example above the key would have `rate` set to 100 and `per` set to 30 giving an effective rate limit of 3.33rps. This has now been corrected.
    +Prior to Tyk 5.4.0 there was a long-standing bug in the calculation of the effective rate limit applied to the key where Tyk would combine the highest `rate` and highest `per` from the policies applied to the key, so for the example above the key would have `rate` set to 100 and `per` set to 30 giving an effective rate limit of 3.33rps. This has now been corrected. For more details, see the [release notes](link-to-the-specific-release-notes).
     {{< /note >}}
     
    Suggestion importance[1-10]: 7

    Why: Adding a direct link to the release notes enhances the documentation by providing readers with easy access to more detailed information, improving user experience and understanding.

    7
    User guidance
    Mention backward compatibility and provide user guidance for configuration review upon upgrading

    To avoid potential confusion, explicitly mention that the change is backward
    compatible or provide guidance on what steps, if any, users should take when
    upgrading.

    tyk-docs/content/product-stack/tyk-gateway/release-notes/version-5.4.md [312]

    -Note that this corrected logic is applied when access keys are presented in API requests. If you are applying multiple policies to keys, there may be a change in the effective rate limit when using Tyk 5.4.0 compared with pre-5.4.0 versions.
    +Note that this corrected logic is applied when access keys are presented in API requests and is backward compatible. Users applying multiple policies to keys should review their configurations to ensure desired operation under the new logic. There may be a change in the effective rate limit when using Tyk 5.4.0 compared with pre-5.4.0 versions.
     
    Suggestion importance[1-10]: 7

    Why: Providing guidance on backward compatibility and advising users to review configurations is valuable for ensuring smooth transitions and preventing potential issues during upgrades.

    7
    Clarity
    Clarify the explanation of how the highest effective rate limit is now applied

    To enhance clarity, consider rephrasing to explicitly state that the highest
    effective rate limit is now correctly applied, avoiding the previous error of
    combining the highest rate and per values incorrectly.

    tyk-docs/content/product-stack/tyk-gateway/release-notes/version-5.4.md [310]

    -With the fix applied in Tyk 5.4.0, the Gateway will now apply the highest effective rate to the key - so in this example, the key would take the rate limit from policy B: `rate = 100` and `per = 10` (10rps).
    +With the fix applied in Tyk 5.4.0, the Gateway now correctly applies the highest effective rate limit. For example, if policy A has a rate limit of 3rps and policy B has 10rps, the key will use the 10rps from policy B, avoiding the previous error of incorrectly combining the highest `rate` and `per` values.
     
    Suggestion importance[1-10]: 6

    Why: The suggestion improves clarity by explicitly stating the correct application of the highest effective rate limit, which helps prevent misunderstandings about the changes made.

    6

    Copy link

    netlify bot commented Aug 20, 2024

    PS. Pls add /docs/nightly to the end of url

    Name Link
    🔨 Latest commit a20b498
    🔍 Latest deploy log https://app.netlify.com/sites/tyk-docs/deploys/66c4b4b506fdf20008587e24
    😎 Deploy Preview https://deploy-preview-5286--tyk-docs.netlify.app
    📱 Preview on mobile
    Toggle QR Code...

    QR Code

    Use your smartphone camera to open QR code link.

    To edit notification comments on pull requests, go to your Netlify site configuration.

    @buger buger merged commit 2cd0275 into release-5.5 Aug 20, 2024
    9 checks passed
    @buger buger deleted the merge/release-5.5/a63f1ba21f9cfcf4084aa28e25ac5e4195b7821f branch August 20, 2024 15:24
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants